Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Opt in to remaining Rails 7.1 defaults #30332

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mjankowski
Copy link
Contributor

I believe that the remainder of the options which were not already enabled here (in the file deleted in this PR) are safe to enable at this point, and we can flip over to the 7.1 defaults.

However - I know we also want to be deliberate with how people step through upgrades, and we may want the 4.3 release to include an update to Rails 7.1 (4.2.x is on Rails 7.0) but NOT immediately enable all these defaults.

Vaguely related previous PRs:

Opening this to step through the final remaining changes in this diff, and coordinate the interaction of release timing/versions with when to make this change.

@@ -57,9 +57,7 @@
module Mastodon
class Application < Rails::Application
# Initialize configuration defaults for originally generated Rails version.
config.load_defaults 7.0

config.active_record.marshalling_format_version = 7.1
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was previously set here to work around a config bug where it didn't get set correctly from the new_framework_defaults file. With bump to 7.1 defaults, it isn't needed.

# in 7.0), then you need to configure SHA-256 for Active Record Encryption:
# Rails.application.config.active_record.encryption.hash_digest_class = OpenSSL::Digest::SHA256
#
# If you don't currently have data encrypted with Active Record encryption, you can disable this setting to
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are effectively in this scenario -- we have the recently enabled otp AR encryption column, but that has only existed on 7.1 and has never been in a release, so I think we are in the SHA256 case described above, and having the framework default of not backwards-supporting SHA1 is acceptable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have thought that as well, but:

May 31 14:59:06 mastodon bundle[993665]: E, [2024-05-31T14:59:06.438057 #993665] ERROR -- : [64fdf096-f096-476a-a51b-9cc20e91919a]
May 31 14:59:06 mastodon bundle[993665]: [64fdf096-f096-476a-a51b-9cc20e91919a] ActiveRecord::Encryption::Errors::Decryption (ActiveRecord::Encryption::Errors::Decryption):
May 31 14:59:06 mastodon bundle[993665]: [64fdf096-f096-476a-a51b-9cc20e91919a]
May 31 14:59:06 mastodon bundle[993665]: [64fdf096-f096-476a-a51b-9cc20e91919a] app/controllers/concerns/auth/two_factor_authentication_concern.rb:32:in `valid_otp_attempt?'
May 31 14:59:06 mastodon bundle[993665]: [64fdf096-f096-476a-a51b-9cc20e91919a] app/controllers/concerns/auth/two_factor_authentication_concern.rb:74:in `authenticate_with_two_factor_via_otp'
May 31 14:59:06 mastodon bundle[993665]: [64fdf096-f096-476a-a51b-9cc20e91919a] app/controllers/concerns/auth/two_factor_authentication_concern.rb:51:in `authenticate_with_two_factor'
May 31 14:59:06 mastodon bundle[993665]: [64fdf096-f096-476a-a51b-9cc20e91919a] lib/mastodon/rack_middleware.rb:9:in `call'

I'm not completely sure this is because of this particular setting, though.

# replicas will not be able to deserialize `BigDecimal` arguments from this
# serializer. Therefore, this setting should only be enabled after all replicas
# have been successfully upgraded to Rails 7.1.
# Rails.application.config.active_job.use_big_decimal_serializer = true
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one needs consideration as to what the impact would be of enabling now -vs- waiting for a proper 4.3 release running Rails 7.1 first (and then enable this in a future release).

#
# In Rails 7.1, the new default is `:json_allow_marshal` which serializes and
# deserializes with `ActiveSupport::JSON`, but can fall back to deserializing
# with `Marshal` so that legacy messages can still be read.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this fallback behavior helps us.

#
# If you are performing a rolling deploy of a Rails 7.1 upgrade, wherein servers
# that have not yet been upgraded must be able to read messages from upgraded
# servers, first deploy without changing the serializer, then set the serializer
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is another one where we may want a 4.3 release running rails 7.1 first, and then make this change after.

# not yet been upgraded must be able to read messages from upgraded servers,
# leave this optimization off on the first deploy, then enable it on a
# subsequent deploy.
# Rails.application.config.active_support.use_message_serializer_for_metadata = true
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another one - maybe should wait for post-7.1 release.

@renchap renchap requested a review from a team May 22, 2024 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants